Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#4051] Use new JSON editor #4407

Merged
merged 11 commits into from
Jul 15, 2024
Merged

[#4051] Use new JSON editor #4407

merged 11 commits into from
Jul 15, 2024

Conversation

Viicos
Copy link
Contributor

@Viicos Viicos commented Jun 18, 2024

Closes #4424

Changes

Update builder
Use new editor in various places

Checklist

Check off the items that are completed or not relevant.

  • Impact on features

    • Checked copying a form
    • Checked import/export of a form
    • Config checks in the configuration overview admin page
    • Problem detection in the admin email digest is handled
  • Release management

    • I have labelled the PR as "needs-backport" accordingly
  • I have updated the translations assets (you do NOT need to provide translations)

    • Ran ./bin/makemessages_js.sh
    • Ran ./bin/compilemessages_js.sh
  • Commit hygiene

    • Commit messages refer to the relevant Github issue
    • Commit messages explain the "why" of change, not the how

@Viicos Viicos marked this pull request as draft June 18, 2024 12:24
@Viicos
Copy link
Contributor Author

Viicos commented Jun 18, 2024

Not having the ability to specify rows/columns is very inconvenient, I wonder if we should simply support cols/rows props and multiply them by a fixed amount of pixels (i.e. 19px for height) to then get the appropriate CSS height/width

Copy link

codecov bot commented Jun 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.57%. Comparing base (ac6a7c2) to head (07bdb8c).
Report is 514 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4407   +/-   ##
=======================================
  Coverage   96.57%   96.57%           
=======================================
  Files         719      719           
  Lines       24089    24090    +1     
  Branches     2863     2863           
=======================================
+ Hits        23263    23264    +1     
  Misses        562      562           
  Partials      264      264           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Viicos Viicos force-pushed the feature/4051-monaco branch 2 times, most recently from d1e1af9 to d1134be Compare June 20, 2024 08:34
@sergei-maertens sergei-maertens force-pushed the feature/4051-monaco branch 5 times, most recently from 0fe115e to cba2fd9 Compare July 12, 2024 13:36
@sergei-maertens sergei-maertens marked this pull request as ready for review July 12, 2024 13:39
It takes any json, as primitives or arrays are also valid JSON logic.
Upgraded the builder to a version that uses the same
monaco-json-editor version to prevent different library
versions being included in the build.

This adds the explicit dependency on the monaco editor,
instead of relying on it being provided through
formio-builder.
These components now use the underlying monaco-json-editor to edit
and/or preview the JSON data being manipulated. This fixes the annoying
autoformatting-while-typing and adds syntax highlighting, plus support
for dark/light theme.

Co-authored-by: @Viicos
Sizing is based on number of rows/cols desired and set through a custom
CSS property on the respective components.
* Moved around the position of widgets a bit, and declutter
* Instead of a toggle link, there's now a toggle icon
* Instead of expand/collapse, replace the editor UI with the Json widget
  and vice versa on toggle
* Ensure that icons have accessible labels when a title is provided
Added a helper to put (JSON) expressions in the editors so that the
user interaction can be simulated. For speed reasons, it uses copy and
paste. Solution is taken from microsoft/playwright#14126
It leads to duplicate labels where the icon is used as a tooltip,
instead we can provide the prop/attribute in places where it's actually
waranted.

This refactors the toggle icon so that the message, behaviour and a11y
impact is contained in a single place.
@sergei-maertens sergei-maertens force-pushed the feature/4051-monaco branch 6 times, most recently from 5a35d67 to 4f62faa Compare July 12, 2024 17:05
@sergei-maertens sergei-maertens force-pushed the feature/4051-monaco branch 5 times, most recently from 59e7a9b to d453cfb Compare July 12, 2024 19:51
Writing to and reading from monaco editors is a bit more
convoluted than making simple assertions on textareas.

The helpers should be re-usable for any place that uses
the monaco-json-editor.
I could not for the life of me get this to work, after spending 4 hours
on it trying various permutations of workarounds:

* using a textarea or div that is contenteditable to grab the code from,
  via page.evaluate. Those actually all worked, but the problem wasn't
  getting the code/JSON on the clipboard - navigator.clipboard
  actually works just fine
* trying various combinations to get the code in the editor to clear:
  CTRL+A, CTRL+End (to end of document) followed by CTRL+Shift+Home to
  select everything from end to beginning of document, followed by
  Delete key to delete it -> can't get this selection to apply,
  with or without delays provided in keyboard.press

I've added numerous screenshots and compared them to what Chromium
browsers do, and that is different - the select-all-and-then-delete
to get rid of the existing content just doesn't seem to work. I don't
know if the editor is not properly initialized or events are not
properly dispatched, but I've given up at this point.

So, warning other users via an exception that this won't work on webkit
to save them the same debugging hassle and adding a skip helper for
these tests should still give us the coverage on chromium and firefox,
and manual testing on webkit will have to make do.
@sergei-maertens sergei-maertens merged commit bdebf25 into master Jul 15, 2024
32 checks passed
@sergei-maertens sergei-maertens deleted the feature/4051-monaco branch July 15, 2024 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate admin JSON components to Monaco editor
2 participants